New ABI encoder #2704

chriseth · 2017-08-07T14:58:58Z

This is disabled for now (waiting for "experimental pragma").

Depends on: #2701 , #2700 , #2690

part of #2433

axic · 2017-08-08T14:35:25Z

What experimental pragma to use?

abiEncoderV2 ?

axic · 2017-08-09T22:56:05Z

libsolidity/codegen/CompilerUtils.h

@@ -103,6 +103,12 @@ class CompilerUtils
 		bool _encodeAsLibraryTypes = false
 	);

+	void abiEncode(


Should have some documentation above.

axic · 2017-08-09T22:56:32Z

libsolidity/codegen/ABIFunctions.cpp

+{
+	// This throws an exception and thus might cause immediate termination, but hey,
+	// it's a failed assertion anyway :-)
+//TODO	solAssert(m_requestedFunctions.empty(), "Forgot to call ``requestedFunctions()``.");


What about this here?

axic · 2017-08-10T15:59:28Z

Can you please rebase?

chriseth · 2017-08-10T16:36:14Z

This allows multi-dimensional arrays in interfaces unconditionally, i.e. now only if the experimental mode is turned on. This is more or less required because it is a type feature.

I don't think this is harmful, because it will only cause an "unimplemented feature" exception in the code generator. I don't think it changes any interfaces, but I'm not 100% positive on that.

axic · 2017-08-10T16:39:56Z

libsolidity/ast/Types.cpp

@@ -1528,8 +1528,6 @@ TypePointer ArrayType::interfaceType(bool _inLibrary) const
 	TypePointer baseExt = m_baseType->interfaceType(_inLibrary);
 	if (!baseExt)
 		return TypePointer();
-	if (m_baseType->category() == Category::Array && m_baseType->isDynamicallySized())


Can you fold this change into another commit or rename the commit message?

axic · 2017-08-10T18:26:20Z

libsolidity/codegen/ABIFunctions.cpp

+		"_to_" +
+		_to.identifier() +
+		(_encodeAsLibraryTypes ? "_lib" : "") +
+		(_compacted ? "_comp" : "");


_compact?

chriseth · 2017-08-11T09:52:10Z

Ok, hopefully no failures anymore now.

axic · 2017-08-11T14:53:08Z

Fails on:

/home/travis/build/ethereum/solidity/libsolidity/codegen/ArrayUtils.cpp(290): fatal error in "void dev::solidity::ArrayUtils::copyArrayToMemory(const dev::solidity::ArrayType &, bool) const": std::exception: Nested dynamic arrays not implemented here.
/home/travis/build/ethereum/solidity/test/RPCSession.cpp(324): last checkpoint

axic · 2017-08-11T15:01:31Z

test/libsolidity/SolidityEndToEndTest.cpp

 	BOOST_REQUIRE_EQUAL(m_logs[0].topics.size(), 1);
 	BOOST_CHECK_EQUAL(m_logs[0].topics[0], dev::keccak256(string("Deposit(uint256,bytes,uint256)")));
 }

 BOOST_AUTO_TEST_CASE(event_really_lots_of_data_from_storage)
 {
 	char const* sourceCode = R"(
+		pragma experimental ABIEncoderV2;


Is this necessary here?

@chriseth I think this is still added to the wrong test here.

Okay reordered this in the commits as it was confusing.

axic · 2017-08-11T15:03:37Z

I haven't reviewed the actual encoder yet, but the rest (bar the above) seems ok.

axic · 2017-08-14T08:27:29Z

libsolidity/codegen/ABIFunctions.cpp

+			break;
+		}
+		case Type::Category::Contract:
+			templ("body", "cleaned := " + cleanupFunction(IntegerType(120, IntegerType::Modifier::Address)) + "(value)");


Should be 160, Address, though I guess the bits is ignored if it is an address. It would make sense adding another constructor in that case.

axic · 2017-08-14T08:27:50Z

libsolidity/codegen/ABIFunctions.cpp

+					toCategory == Type::Category::Integer ||
+					toCategory == Type::Category::Contract,
+				"");
+				IntegerType const addressType(0, IntegerType::Modifier::Address);


Same as above, 160, Address I guess.

axic · 2017-08-14T08:28:30Z

libsolidity/codegen/ABIFunctions.cpp

+					Whiskers("converted := <shiftLeft>(<clean>(value))")
+					("shiftLeft", shiftLeftFunction(256 - toBytesType.numBytes() * 8))
+					("clean", cleanupFunction(_from))
+					.render();


The formatting on these is a bit misleading, probably this is more readable:

Whiskers(...) ("shiftLeft",...) ... .render()

axic · 2017-08-14T08:28:52Z

libsolidity/codegen/ABIFunctions.cpp

+		}
+		case Type::Category::Function:
+		{
+			solAssert(false, "Cleanup should not be called for function types.");


This is conversion, not cleanup.

axic · 2017-08-14T08:30:38Z

libsolidity/codegen/ABIFunctions.cpp

+	}
+}
+
+string ABIFunctions::combineExternalFunctionIdFunction()


Can you move this next to cleanupCombinedExternalFunctionId?

axic · 2017-08-14T08:49:39Z

libsolidity/codegen/ABIFunctions.cpp

+		_from.identifier() +
+		"_to_" +
+		_to.identifier() +
+		(_encodeAsLibraryTypes ? "_lib" : "");


Probably no point truncating it from _library to _lib.

axic · 2017-08-14T09:47:44Z

libsolidity/codegen/ABIFunctions.cpp

+					<#word>
+						mstore(add(pos, <offset>), <wordValue>)
+					</word>
+					end := add(pos, <overallSize>)


You could increment the pos in-place in the loop and use end := add(pos, 64).

I think this way it is better for the optimizer.

axic · 2017-08-14T09:49:25Z

libsolidity/codegen/ABIFunctions.cpp

+		if (_to.isDynamicallySized())
+		{
+			Whiskers templ(R"(
+				function <functionName>(pos) -> end {


Hm, this one returns the occupied memory size but the other encoding functions below don't?

It returns the "new memory pointer" if and only if it is dynamically sized. You can take a look at how the functions are called, it is consistent and should be checked by the julia analyzer.

axic · 2017-08-14T09:51:05Z

libsolidity/codegen/ABIFunctions.cpp

+				function <functionName>(src, dst, length) {
+					calldatacopy(dst, src, length)
+					// clear end
+					mstore(add(dst, length), 0)


Why is there an extra slot at the end?

It just removes potential garbage. The routine might write to more memory than it actually "returns" in the end. This is documented in the header.

It just feels weird this is not handled in the caller, why would it ask for more if the entire slot is cleared?

@chriseth this question wasn't sorted out

The ABI specification requires everything to be padded with zeros to multiples of 32 bytes. The calldatacopy only copies the actual length. This line performs the padding, especially in the case when the memory was pre-occupied by something else.

axic · 2017-08-14T09:53:51Z

libsolidity/codegen/ABIFunctions.cpp

+			switch (_type.location())
+			{
+			case DataLocation::CallData:
+				solAssert(false, "called regular array length function on calldata array");


Shouldn't this be possible?

No, because calldata arrays are stored as (pos, length), so you cannot retrieve the length from pos alone.

axic · 2017-08-14T10:00:00Z

libsolidity/codegen/ABIFunctions.cpp

+		// because the encoding is position-independent, but we have to check that.
+		Whiskers templ(R"(
+			function <functionName>(start, length, pos) -> end {
+				<storeLength>


Add "// might update pos".

chriseth added the in progress label Aug 7, 2017

axic mentioned this pull request Aug 8, 2017

Store experimental flag in metadata CBOR #2712

Merged

axic reviewed Aug 9, 2017

View reviewed changes

chriseth force-pushed the newEncoder branch from ad6baa0 to c6947f0 Compare August 10, 2017 16:34

axic reviewed Aug 10, 2017

View reviewed changes

chriseth force-pushed the newEncoder branch from c6947f0 to 676cd23 Compare August 11, 2017 09:44

axic force-pushed the newEncoder branch from 676cd23 to 6b44b63 Compare August 11, 2017 14:57

axic reviewed Aug 11, 2017

View reviewed changes

chriseth force-pushed the newEncoder branch from 6b44b63 to 69d262d Compare August 11, 2017 15:45

chriseth mentioned this pull request Aug 11, 2017

[WIP] Struct encoder #2433

Closed

3 tasks

chriseth force-pushed the newEncoder branch from 69d262d to e48f58a Compare August 11, 2017 17:02

axic added the nextrelease label Aug 11, 2017

chriseth force-pushed the newEncoder branch from e48f58a to 2eb6d66 Compare August 11, 2017 18:53

axic reviewed Aug 14, 2017

View reviewed changes

chriseth added 4 commits August 14, 2017 12:44

Allow multi-dimensional arrays in interfaces.

77b2655

Helpers.

bda410b

New ABI encoder.

42fe8a2

Interface for new ABI encoder.

4630b33

chriseth force-pushed the newEncoder branch from 2eb6d66 to f0f2e6a Compare August 14, 2017 10:44

chriseth added 4 commits August 14, 2017 13:11

ABI encoder tests.

38446a9

Remove unused magic global feature.

ee6f56d

Experimental feature switch for ABI encoder.

d1ad62f

Test both encoders.

63b556b

axic force-pushed the newEncoder branch from f0f2e6a to 63b556b Compare August 14, 2017 12:12

axic approved these changes Aug 14, 2017

View reviewed changes

chriseth merged commit 2411f5d into develop Aug 14, 2017

chriseth removed in progress labels Aug 14, 2017

axic deleted the newEncoder branch August 15, 2017 09:46

n0izn0iz mentioned this pull request Aug 7, 2024

[chain] ABI-like support for Realm / Package requests / responses gnolang/gno#1842

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ABI encoder #2704

New ABI encoder #2704

chriseth commented Aug 7, 2017

axic commented Aug 8, 2017 •

edited

Loading

axic Aug 9, 2017

axic Aug 9, 2017

axic commented Aug 10, 2017

chriseth commented Aug 10, 2017

axic Aug 10, 2017

axic Aug 14, 2017

axic Aug 10, 2017

chriseth commented Aug 11, 2017

axic commented Aug 11, 2017

axic Aug 11, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic commented Aug 11, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017 •

edited

Loading

chriseth Aug 14, 2017

axic Aug 14, 2017

axic Aug 14, 2017

chriseth Aug 14, 2017

axic Aug 14, 2017

chriseth Aug 14, 2017

axic Aug 14, 2017

axic Aug 21, 2017

chriseth Aug 23, 2017 •

edited

Loading

axic Aug 14, 2017

chriseth Aug 14, 2017

axic Aug 14, 2017

New ABI encoder #2704

New ABI encoder #2704

Conversation

chriseth commented Aug 7, 2017

axic commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Aug 10, 2017

chriseth commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Aug 11, 2017

axic commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Aug 8, 2017 •

edited

Loading

axic Aug 14, 2017 •

edited

Loading

chriseth Aug 23, 2017 •

edited

Loading